[aarch64] add target feature outline-atomics #83655

sebpop · 2021-03-29T18:29:37Z

Enable outline-atomics by default as enabled in clang by the following commit
https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11

Performance improves by several orders of magnitude when using the LSE instructions
instead of the ARMv8.0 compatible load/store exclusive instructions.

Tested on Graviton2 aarch64-linux with
x.py build && x.py install && x.py test

rust-highfive · 2021-03-29T18:29:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bors · 2021-04-03T02:21:31Z

☔ The latest upstream changes (presumably #81866) made this pull request unmergeable. Please resolve the merge conflicts.

sebpop · 2021-04-13T14:16:34Z

Ping.

sebpop · 2021-04-21T20:11:16Z

Ping.

joshtriplett · 2021-04-30T03:03:28Z

@sebpop This seems reasonable. Can you add a codegen test that ensures a reference to the outlined atomics is actually generated as expected?

joshtriplett · 2021-04-30T03:12:02Z

Also, could you enable this for aarch64-unknown-linux-musl as well (and make sure it links correctly)?

joshtriplett · 2021-04-30T20:18:27Z

#84764 should help with this.

joshtriplett · 2021-04-30T22:56:36Z

Looks great! r=me if it passes CI.

@bors delegate+

bors · 2021-04-30T22:56:38Z

✌️ @sebpop can now approve this pull request

joshtriplett · 2021-05-01T03:44:52Z

@bors r+

bors · 2021-05-01T03:44:53Z

📌 Commit a32938b has been approved by joshtriplett

…htriplett [aarch64] add target feature outline-atomics Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test

bors · 2021-05-01T15:12:14Z

⌛ Testing commit a32938b with merge d7f747914deb61e11a59af5c049f64384c2e4a5c...

bors · 2021-05-01T15:36:18Z

💔 Test failed - checks-actions

sebpop · 2021-05-11T23:23:04Z

Is there something I can help with to make the CI tester pass?

sebpop · 2021-05-17T23:19:59Z

@bors r+

bors · 2021-05-17T23:20:01Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Override clone_from for some types #85176

workingjubilee · 2021-09-29T01:04:29Z

Hm, to_llvm_feature is for canonicalizing Rust feature names to LLVM feature names so that user input only has to use one feature name invariantly of codegen backend. However, currently we also expose a pass-through to LLVM-only features. This changeset happens to work here because that function is invariantly over all input, including both user feature input and the compiler's target definition input, and both Rust feature names and LLVM feature names. It including a version check is just because in LLVM v14, LLVM intends to split the definition of a feature in two, so now the official Rust feature means two LLVM features.

As a result this latest version is somewhat off in this context, especially because it technically silently ignores user input. I don't anticipate that to be a huge problem but this rectifying check should be kept logically separate instead of being included with the canonicalizing match block, including with a note to why (i.e. "we added this to our target definition before our earliest LLVM supported it").

Alternatively the appropriate tests can acquire min LLVM version notes or the like.

sebpop · 2021-09-29T18:40:10Z

I was not able to specify in aarch64 target definition that the outline-atomics feature should be added only for llvm >= 12, i.e., something like:

            features: if llvm_version >= 12 {                                                                                                                                                                      
                "+outline-atomics".to_string()                                                                                                                                                                     
            },

The problem is that llvm_version is not available in compiler/rustc_target.
llvm_version only becomes available in compiler/rustc_codegen_llvm/ that gets linked with llvm libs.
That's why I tried to remove outline-atomics feature when the llvm compiler does not support it during normalization of target features.

I am reverting these changes to the previous patch that passed CI tests with requiring min_llvm_version 12 for two of the tests.

workingjubilee · 2021-09-29T18:56:51Z

Yes, I think the codegen "target feature" infrastructure needs to be reworked to make this kind of change easier, but I see no reason to put that on your shoulders: I just want the "noisy" failure mode instead of the silent one.

sebpop · 2021-09-29T19:07:31Z

I just want the "noisy" failure mode instead of the silent one.

If rust is configured with llvm < 12 on aarch64-linux it will output the warn message '+outline-atomics' is not a recognized feature for this target (ignoring feature) for all compiles. I am thinking that could be annoying.

workingjubilee · 2021-09-29T21:37:27Z

Indeed, and if they inputted it manually expecting it to work, but hadn't yet upgraded their LLVM version, silently ignoring it, it would not provide user feedback to upgrade their LLVM version, which I anticipate would be 300% more vexing and harder to diagnose.

I expect, at the moment, the cleanest possible solution here that satisfies all goals, which are:

no nuisance to existing users who upgrade to the latest nightly but have a pinned LLVM
no ignoring explicit user requests silently
landing this feature on a "by default" basis

is to:

remove it from the target definition
do a version / arch check / OS check and then add the feature to aarch64-linux on LLVM 12+
add a FIXME comment that it can go in the target definition when our earliest LLVM is 12

Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test

workingjubilee · 2021-10-03T23:20:52Z

Excellent, thank you. Let's try this again.
@bors r+

bors · 2021-10-03T23:20:54Z

📌 Commit 0f9f241 has been approved by workingjubilee

…kingjubilee [aarch64] add target feature outline-atomics Enable outline-atomics by default as enabled in clang by the following commit https://reviews.llvm.org/rGc5e7e649d537067dec7111f3de1430d0fc8a4d11 Performance improves by several orders of magnitude when using the LSE instructions instead of the ARMv8.0 compatible load/store exclusive instructions. Tested on Graviton2 aarch64-linux with x.py build && x.py install && x.py test

…ingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#83655 ([aarch64] add target feature outline-atomics) - rust-lang#87091 (implement advance_(back_)_by on more iterators) - rust-lang#88451 (Fix an ICE caused by type mismatch errors being ignored) - rust-lang#88452 (VecDeque: improve performance for From<[T; N]>) - rust-lang#89400 (Improve wording of `map_or_else` docs) - rust-lang#89407 (Recommend running `cargo clean` in E0514 output) - rust-lang#89443 (Include the length in BTree hashes) - rust-lang#89444 (rustdoc: use slice::contains instead of open-coding it) - rust-lang#89447 (Improve error message for missing angle brackets in `[_]::method`) - rust-lang#89453 (Consistently use 'supertrait'.) - rust-lang#89483 (Practice diagnostic message convention) - rust-lang#89500 (Fix ICE with buffered lint referring to AST node deleted by everybody_loops) - rust-lang#89508 (Stabilize `const_panic`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…nly, r=workingjubilee Restrict the aarch64 outline atomics test to Linux The test was introduced in rust-lang#83655, which enables the `outline-atomics` feature for aarch64-unknown-linux-* but not for any other aarch64 targets. The test did not check for Linux causing test failures on aarch64-apple-darwin. r? `@workingjubilee`

ehuss · 2021-11-04T03:49:50Z

@sebpop This PR broke building for aarch64-unknown-linux using -Zbuild-std. I was wondering if you or anyone else here can provide some guidance on where to look at how to fix it.

I don't have access to an aarch64 system, so I'm testing on x86_64-unknown-linux-gnu cross-compiling to aarch64-unknown-linux-gnu. I am using aarch64-unknown-linux-gnueabi-gcc as a linker. Running cargo build -Zbuild-std --target aarch64_unknown-linux-gnu on a hello world project yields several linking errors with atomics:

  = note: /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_add::hd35126eded0fd48a':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2392: undefined reference to `__aarch64_ldadd8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2390: undefined reference to `__aarch64_ldadd8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2389: undefined reference to `__aarch64_ldadd8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2391: undefined reference to `__aarch64_ldadd8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2393: undefined reference to `__aarch64_ldadd8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_sub::h509cbca9066c2819':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2408: undefined reference to `__aarch64_ldadd8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2406: undefined reference to `__aarch64_ldadd8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2405: undefined reference to `__aarch64_ldadd8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2407: undefined reference to `__aarch64_ldadd8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2409: undefined reference to `__aarch64_ldadd8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_sub::hf59db215e96f1bc4':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2408: undefined reference to `__aarch64_ldadd4_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2406: undefined reference to `__aarch64_ldadd4_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2405: undefined reference to `__aarch64_ldadd4_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2407: undefined reference to `__aarch64_ldadd4_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2409: undefined reference to `__aarch64_ldadd4_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h11448fb64a88a78e':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp8_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp8_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp8_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp8_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp8_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h449dcb7a0edc4e35':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp4_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp4_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp4_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp4_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp4_acq_rel'
          /checkout/obj/foo/target/aarch64-unknown-linux-gnu/debug/deps/libstd-89e94665a5a1cb02.rlib(std-89e94665a5a1cb02.std.68c03dd8-cgu.13.rcgu.o): In function `core::sync::atomic::atomic_swap::h6031e09dc0173e7a':
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2376: undefined reference to `__aarch64_swp1_relax'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2374: undefined reference to `__aarch64_swp1_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2373: undefined reference to `__aarch64_swp1_acq'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2375: undefined reference to `__aarch64_swp1_acq_rel'
          /root/.rustup/toolchains/nightly-2021-10-05-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/sync/atomic.rs:2377: undefined reference to `__aarch64_swp1_acq_rel'
          collect2: error: ld returned 1 exit status

It is not immediately clear to me what the difference is between -Zbuild-std and a normal build done by x.py. One theory I have is that -Zbuild-std doesn't use the c feature of compiler-builtins, but I'm uncertain (it's a bit hard to test). Any ideas?

sebpop · 2021-11-04T14:26:46Z

The missing symbols are distributed by llvm in compiler-rt and by gcc in libgcc.
It looks like your program does not link against one of these two libraries.
As you are cross-compiling, you will need the aarch64 version of compiler-rt or libgcc.

Amanieu · 2021-11-23T12:48:02Z

One theory I have is that -Zbuild-std doesn't use the c feature of compiler-builtins, but I'm uncertain (it's a bit hard to test). Any ideas?

I just hit this myself. You need to pass -Zbuild-std-features=compiler-builtins-c and then set the COMPILER_RT_ROOT environment variable. It's a bit of a hassle so I just disabled outlined atomics with RUSTFLAGS=-Ctarget-feature=-outline-atomics`.

rust-highfive assigned matthewjasper Mar 29, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 29, 2021

sebpop force-pushed the arm64-outline-atomics branch 3 times, most recently from 93b958b to 003065a Compare April 5, 2021 17:17

sebpop force-pushed the arm64-outline-atomics branch from 003065a to a32938b Compare April 30, 2021 21:33

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2021

Dylan-DPC-zz mentioned this pull request May 1, 2021

Rollup of 6 pull requests #84788

Closed

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 1, 2021

sebpop force-pushed the arm64-outline-atomics branch from 79b65db to e5ca2ca Compare September 29, 2021 18:40

sebpop force-pushed the arm64-outline-atomics branch from e5ca2ca to 0f9f241 Compare September 30, 2021 23:37

Manishearth mentioned this pull request Oct 4, 2021

Rollup of 14 pull requests #89525

Closed

Manishearth mentioned this pull request Oct 4, 2021

Rollup of 14 pull requests #89529

Closed

workingjubilee mentioned this pull request Oct 4, 2021

Rollup of 13 pull requests #89530

Merged

bors merged commit 4e9cf04 into rust-lang:master Oct 4, 2021

rustbot added this to the 1.57.0 milestone Oct 4, 2021

workingjubilee mentioned this pull request Oct 5, 2021

Use the 64b inner:monotonize() implementation not the 128b one for aarch64 #88651

Merged

hkratz mentioned this pull request Oct 15, 2021

Restrict the aarch64 outline atomics test to Linux #89902

Merged

hkratz mentioned this pull request Oct 18, 2021

Undefined reference to getauxval in function init_have_lse_atomics when compiling to nightly aarch64-unknown-linux-musl #89626

Open

12101111 mentioned this pull request Nov 3, 2021

undefined reference errors on aarch64 rust-lang/wg-cargo-std-aware#74

Closed

workingjubilee mentioned this pull request Mar 13, 2023

+outline-atomics violates "core shall not depend on libc" #109064

Closed

jyn514 mentioned this pull request Jun 25, 2023

Remove +outline-atomics from arm64 linux rust-lang/compiler-team#643

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aarch64] add target feature outline-atomics #83655

[aarch64] add target feature outline-atomics #83655

sebpop commented Mar 29, 2021

rust-highfive commented Mar 29, 2021

bors commented Apr 3, 2021

sebpop commented Apr 13, 2021

sebpop commented Apr 21, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

bors commented Apr 30, 2021

joshtriplett commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

This comment has been minimized.

bors commented May 1, 2021

sebpop commented May 11, 2021

sebpop commented May 17, 2021

bors commented May 17, 2021

workingjubilee commented Sep 29, 2021 •

edited

Loading

sebpop commented Sep 29, 2021

workingjubilee commented Sep 29, 2021

sebpop commented Sep 29, 2021

workingjubilee commented Sep 29, 2021 •

edited

Loading

workingjubilee commented Oct 3, 2021

bors commented Oct 3, 2021

ehuss commented Nov 4, 2021

sebpop commented Nov 4, 2021

Amanieu commented Nov 23, 2021

[aarch64] add target feature outline-atomics #83655

[aarch64] add target feature outline-atomics #83655

Conversation

sebpop commented Mar 29, 2021

rust-highfive commented Mar 29, 2021

bors commented Apr 3, 2021

sebpop commented Apr 13, 2021

sebpop commented Apr 21, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

joshtriplett commented Apr 30, 2021

bors commented Apr 30, 2021

joshtriplett commented May 1, 2021

bors commented May 1, 2021

bors commented May 1, 2021

This comment has been minimized.

bors commented May 1, 2021

sebpop commented May 11, 2021

sebpop commented May 17, 2021

bors commented May 17, 2021

workingjubilee commented Sep 29, 2021 • edited Loading

sebpop commented Sep 29, 2021

workingjubilee commented Sep 29, 2021

sebpop commented Sep 29, 2021

workingjubilee commented Sep 29, 2021 • edited Loading

workingjubilee commented Oct 3, 2021

bors commented Oct 3, 2021

ehuss commented Nov 4, 2021

sebpop commented Nov 4, 2021

Amanieu commented Nov 23, 2021

workingjubilee commented Sep 29, 2021 •

edited

Loading

workingjubilee commented Sep 29, 2021 •

edited

Loading